Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add support for error notifications for Snowpipe #595

Merged
merged 12 commits into from
Jul 13, 2021
Merged

feat: Add support for error notifications for Snowpipe #595

merged 12 commits into from
Jul 13, 2021

Conversation

gouline
Copy link
Contributor

@gouline gouline commented Jul 7, 2021 •
edited
Loading

Adding support for error notifications for Snowpipe.

This feature is currently in private preview and must be enabled for the specific account before these changes can work. Once the feature is released (not sure when), it should work for everybody.

The setup consists of a new outbound type of notification integrations (only works with AWS SQS for now):

CREATE NOTIFICATION INTEGRATION <integration_name>
  ENABLED = true
  TYPE = QUEUE
  NOTIFICATION_PROVIDER = AWS_SQS
  DIRECTION = OUTBOUND
  AWS_SQS_ARN = '<queue_arn>'
  AWS_SQS_ROLE_ARN = '<iam_role_arn>'

And the new ERROR_INTEGRATION parameter for pipes:

CREATE PIPE <name>
  [ AUTO_INGEST = TRUE | FALSE  ]
  [ AWS_SNS_TOPIC = <string> ]
  ERROR_INTEGRATION = <integration_name>
  AS <copy_statement>

ALTER PIPE <name> SET ERROR_INTEGRATION = <integration_name>;

Test Plan

  • acceptance tests

References

@gouline gouline requested a review from a team as a code owner July 7, 2021 11:27
@gouline gouline requested a review from alldoami July 7, 2021 11:27
@gouline gouline changed the title Add support for error notifications for Snowpipe [feat] Add support for error notifications for Snowpipe Jul 7, 2021
@gouline gouline changed the title [feat] Add support for error notifications for Snowpipe feat: Add support for error notifications for Snowpipe Jul 7, 2021
@alldoami
Copy link
Contributor

alldoami commented Jul 7, 2021

Are you going to implement the error_integration feature? Also I think it'd be better if you rename to error_notification.

@gouline
Copy link
Contributor Author

gouline commented Jul 8, 2021 •
edited
Loading

Sorry, I was still figuring out how everything works here 😅 . I think I covered everything now? Please let me know if I missed anything, I'm new to this codebase.

The name error_integration comes from the actual parameter set in CREATE PIPE and ALTER PIPE, see in documentation.

alldoami
alldoami reviewed Jul 8, 2021
View reviewed changes
pkg/resources/pipe_test.go Outdated Show resolved Hide resolved
@alldoami
Copy link
Contributor

alldoami commented Jul 8, 2021

Could you also add a new acceptance test with setting the error_integration 😄

@alldoami
Copy link
Contributor

alldoami commented Jul 8, 2021

Also run make docs

@gouline
Copy link
Contributor Author

gouline commented Jul 9, 2021

Could you also add a new acceptance test with setting the error_integration 😄

Are acceptance tests run on an actual Snowflake account? If so, this might be problematic since the error notifications feature is currently in private preview and Snowflake staff need to enable it for that particular account before setting error_integration parameter would work.

Other stuff is all fixed ✅

@gouline
Copy link
Contributor Author

gouline commented Jul 9, 2021

Something else I missed: I had thought that notification integration needed for this feature was already implemented, but that was only for AZURE_STORAGE_QUEUE and these Snowpipe error notifications only work on AWS for now. So I added an AWS_SQS implementation and corresponding unit tests.

Notification integrations can also be GCP_PUBSUB, but that's outside the scope of this pull request and I will leave it for somebody using GCP to implement 🙂

@gouline
Copy link
Contributor Author

gouline commented Jul 12, 2021

Exposed AWS_SQS_EXTERNAL_ID for notification integration, I think that's everything now. Let me know if there's anything else I need to do before this can get merged.

@alldoami
Copy link
Contributor

/ok-to-test sha=b7f0b08

@alldoami
Copy link
Contributor

Run make docs

@github-actions
Copy link

Integration tests failure for b7f0b08

@alldoami
Copy link
Contributor

I'm looking into turning on the error notifications for our test account, but it looks like there are some other errors...

@gouline
Copy link
Contributor Author

gouline commented Jul 13, 2021

Docs re-generated and the two failing tests should now be fixed ✅

@alldoami
Copy link
Contributor

/ok-to-test sha=479f1ee

@github-actions
Copy link

Integration tests success for 479f1ee

@alldoami alldoami merged commit 90af4cf into Snowflake-Labs:main Jul 13, 2021
@gouline gouline mentioned this pull request Jul 20, 2021
Merged
2 tasks
jtzero pushed a commit to rxrevu/terraform-provider-snowflake that referenced this pull request Aug 19, 2021
@gouline @jtzero
feat: Add support for error notifications for Snowpipe (Snowflake-Lab…
5c0b8d5 
…s#595)
@ghost ghost mentioned this pull request Dec 3, 2021
Merged
@gouline gouline mentioned this pull request Jan 25, 2022
Merged
1 task
anton-chekanov pushed a commit to anton-chekanov/terraform-provider-snowflake that referenced this pull request Jan 25, 2022
@gouline @anton-chekanov
feat: Add support for error notifications for Snowpipe (Snowflake-Lab…
6618a69 
…s#595)
@gouline gouline mentioned this pull request Jan 27, 2022
Merged
1 task
daniepett pushed a commit to daniepett/terraform-provider-snowflake that referenced this pull request Feb 9, 2022
@gouline
feat: Add support for error notifications for Snowpipe (Snowflake-Lab…
2833566 
…s#595)
@snowflake-release-please snowflake-release-please bot mentioned this pull request May 25, 2022
Merged
This was referenced Nov 1, 2022
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alldoami alldoami alldoami approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gouline @alldoami